Refactor query history to handle remote and local#1143
Conversation
| ): Promise<void> { | ||
| const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect); | ||
| if (!this.assertSingleQuery(finalMultiSelect)) { | ||
| if (!this.assertSingleQuery(finalMultiSelect) || finalSingleItem.t !== 'local') { |
There was a problem hiding this comment.
(Optional) We could have a TODO here like we do in other places.
Same with handleShowQueryText, handleViewSarifAlerts etc.
|
|
||
| (finalMultiSelect || [finalSingleItem]).forEach((item) => { | ||
| if (item.status === QueryStatus.InProgress) { | ||
| if (item.status === QueryStatus.InProgress && item.t === 'local') { |
There was a problem hiding this comment.
Might be worth adding a comment here saying that we don't currently support cancellation of remote queries.
There was a problem hiding this comment.
What I meant with this is that it'd be nice to be clear that we're not planning to support cancellation yet vs we're going to do it in the next PR or something like that.
This is a step on the way towards storing remote query history across restarts. This PR adds a `QueryHistoryInfo` type that is a union of two types: `LocalQueryInfo` and `RemoteQueryInfo`. `LocalQueryInfo` used to be called `FullQueryInfo` and `RemoteQueryInfo` is only a skeleton right now. The body will be added later. This PR only introduces it and changes types to make future PRs simpler. Also, `slurp` and `splat` have been moved to the `query-serialization.ts` module.
5daab89 to
39f9c08
Compare
21aa7b8 to
c78802a
Compare
| singleItem: LocalQueryInfo, | ||
| multiSelect: LocalQueryInfo[] |
There was a problem hiding this comment.
| singleItem: LocalQueryInfo, | |
| multiSelect: LocalQueryInfo[] | |
| singleItem: QueryHistoryInfo, | |
| multiSelect: QueryHistoryInfo[] |
| singleItem: LocalQueryInfo, | ||
| multiSelect: LocalQueryInfo[] |
There was a problem hiding this comment.
I think I need to do this:
| singleItem: LocalQueryInfo, | |
| multiSelect: LocalQueryInfo[] | |
| singleItem: QueryHistoryInfo, | |
| multiSelect: QueryHistoryInfo[] |
|
Am I right thinking that I should wait for you to take another pass at the code/comments before I re-review @aeisenberg ? |
…ctor-query-history-info
|
Yes. Going through this again now. |
Also, rename RemoteQueryInfo -> RemoteQueryHistoryItem
charisk
left a comment
There was a problem hiding this comment.
Just some minor suggestions from my previous review (see unresolved comments)
|
I think you're asking to clearly note which commands we plan on supporting for remote queries and which ones we won't. I can add TODOs for the ones we will implement, and regular comments for those we won't. |
| // TODO will support remote queries | ||
| const { finalSingleItem, finalMultiSelect } = this.determineSelection(singleItem, multiSelect); | ||
| if (!this.assertSingleQuery(finalMultiSelect) || finalSingleItem.t !== 'local') { | ||
| if (!this.assertSingleQuery(finalMultiSelect) || (finalSingleItem && finalSingleItem.t !== 'local')) { |
There was a problem hiding this comment.
Any reason you're not doing finalSingleItem?.t !== 'local' like in other places?
There was a problem hiding this comment.
Yes. In this case, if the selected query is remote, then we want this command to do nothing. If there is no selection, then we want to throw an error. It is trying to preserve previous behaviour.
However, in a future PR, I got rid of the throw clause. It's actually really difficult for a user to get into a situation where this command is invoked, but no query is selected. Also, if it does happen, then popping up an error message is a bit annoying.
So, for this PR, I was just planning on maintaining previous behaviour.
This is a step on the way towards storing remote query history across
restarts.
This PR adds a
QueryHistoryInfotype that is a union of two types:LocalQueryInfoandRemoteQueryInfo.LocalQueryInfoused to be calledFullQueryInfoandRemoteQueryInfois only a skeleton right now. The body will be added later. This PR
only introduces it and changes types to make future PRs simpler.
Also,
slurpandsplathave been moved to thequery-serialization.tsmodule.
Checklist
ready-for-doc-reviewlabel there.